Enforce mandatory session verification only for new logins #2811
Conversation
- Creates `AppMigration` base interface as a way to isolate migration logic, app migrations must implement this interface. - Creates `AppMigration01` with the existing logs removal migration and `AppMigration02` with the logic to allow existing sessions to skip verification. - Add `DefaultSessionPreferencesStoreFactory.remove(sessionId)` to allow a ephemeral session store access to exist outside the `SessionScope` for this new migration.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2811 +/- ##
===========================================
+ Coverage 73.43% 73.45% +0.02%
===========================================
Files 1523 1527 +4
Lines 36532 36564 +32
Branches 7042 7044 +2
===========================================
+ Hits 26827 26858 +31
Misses 6026 6026
- Partials 3679 3680 +1 ☔ View full report in Codecov by Sentry. |
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
This also includes creating several abstractions.
| // migrationStore.setApplicationMigrationVersion(0) | ||
| // } | ||
|
|
||
| val orderedMigrations = migrations.sortedBy { it.order } |
There was a problem hiding this comment.
This could be moved as a Class member I think, and so migrations should not be a class member, to avoid any mistake.
There was a problem hiding this comment.
You mean storing it as a property and making migrations just a constructor parameter?
| @@ -68,6 +74,6 @@ class MigrationPresenter @Inject constructor( | |||
| companion object { | |||
| // Increment this value when you need to run the migration again, and | |||
| // add step in the LaunchedEffect above | |||
| const val MIGRATION_VERSION = 1 | |||
| const val MIGRATION_VERSION = 2 | |||
There was a problem hiding this comment.
MIGRATION_VERSION should maybe remove, and use migrations.maxOf { it.order } instead, maybe, again, I think it will reduce the risk of error.
For instance, the new code never write MIGRATION_VERSION to the store as before:
migrationStore.setApplicationMigrationVersion(MIGRATION_VERSION)
|
|
||
| package io.element.android.features.migration.impl.migrations | ||
|
|
||
| interface AppMigration { |
There was a problem hiding this comment.
This could be moved to the api module, so that any module may contribute to migrate their internal state. But maybe keep this for later.
- Make `orderedMigrations` a class property, `migrations` just a constructor parameter to avoid incorrect usages. - Create `lastMigration` property too, use it instead of `MIGRATION_VERSION`.
|
| ) : Presenter<MigrationState> { | ||
| private val orderedMigrations = migrations.sortedBy { it.order } | ||
| private val lastMigration: Int = orderedMigrations.lastOrNull()?.order ?: 0 |
jmartinesp
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
jmartinesp
deleted the
fix/jme/2810-enforce-mandatory-verification-only-for-new-logins
branch
|
OK, I have done some test and it seems to work. Thanks! |
Type of change
Content
AppMigrationbase interface as a way to isolate migration logic, app migrations must implement this interface.AppMigration01with the existing logs removal migration andAppMigration02with the logic to allow existing sessions to skip verification.DefaultSessionPreferencesStoreFactory.remove(sessionId)to allow a ephemeral session store access to exist outside theSessionScopefor this new migration.Motivation and context
Closes #2810.
Tests
Either log in using a previous version of the app and don't verify your device yet, or:
MigrationPresenterto reset the migration version.You should skip the verification flow.
Tested devices
Checklist